PENG-7708-python-sdk-zoneexport#145
Conversation
- Add export() method to REST API and Zone class - Add unit test for zone export - Add simple usage example Allows customers to export zones in BIND format for backup/migration.
|
| print(zone_file) | ||
|
|
||
| # save to a file | ||
| with open("example.com.zone", "w") as f: |
There was a problem hiding this comment.
| with open("example.com.zone", "w") as f: | |
| with open("example.com.txt", "w") as f: |
Usually zone files just have a .txt extension; and zone is a tld, so calling them .zone could be a bit confusing.
There was a problem hiding this comment.
Changed to example.com.txt in line 31.
| errback=errback, | ||
| ) | ||
|
|
||
| def export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
| def export(self, zone, callback=None, errback=None): | |
| def get_zonefile_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
Renamed to get_zonefile_export()
| errback=errback, | ||
| ) | ||
|
|
||
| def initiate_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
| def initiate_export(self, zone, callback=None, errback=None): | |
| def initiate_zonefile_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
Renamed to initiate_zonefile_export()
| errback=errback, | ||
| ) | ||
|
|
||
| def export_status(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
| def export_status(self, zone, callback=None, errback=None): | |
| def status_zonefile_export(self, zone, callback=None, errback=None): |
There was a problem hiding this comment.
Renamed to status_zonefile_export()
|
|
||
| def export(self, zone, callback=None, errback=None): | ||
| """ | ||
| Export zone as BIND-compatible zone file. |
There was a problem hiding this comment.
Could probably say this downloads the zonefile.
| zone=self.zone, callback=callback, errback=errback, **kwargs | ||
| ) | ||
|
|
||
| def export(self, callback=None, errback=None): |
There was a problem hiding this comment.
Can you replace these with a single function that:
- Initialises the export
PUT - Polls the status, until "COMPLETE" or "FAILED" or some timeout hits.
- Calls/returns the download endpoint.
There was a problem hiding this comment.
Enhanced the example with detailed comments explaining the complete workflow. The export() method now handles all three steps automatically: initiate, poll status, and download.
| :return: zone file content as string | ||
| :raises ZoneException: if export fails or times out | ||
| """ | ||
| import time |
There was a problem hiding this comment.
You shouldn't import packages inside functions, can you move this to the top with the other imports?
| self.zone, callback=callback, errback=errback | ||
| ) | ||
|
|
||
| if callback: |
There was a problem hiding this comment.
Is this necessary here? you're already sending get_zonefile_export so any callback will get called above?
| import time | ||
|
|
||
| # Initiate the export | ||
| self._rest.initiate_zonefile_export(self.zone) |
There was a problem hiding this comment.
You should check the response here; make sure it's 200.
If something goes wrong here then we can't tell and it'll start polling for something that will never finish.
| status_response = self._rest.status_zonefile_export(self.zone) | ||
| status = status_response.get("status") | ||
|
|
||
| if status == "COMPLETE": |
There was a problem hiding this comment.
| if status == "COMPLETE": | |
| if status == "COMPLETED": |
| # Note: This endpoint returns raw zone file text, not JSON | ||
| # The transport layer will try to parse it as JSON and fail | ||
| # We catch that exception and extract the raw body text | ||
| from ns1.rest.errors import ResourceException |
There was a problem hiding this comment.
imports need to go on the top of the file
There was a problem hiding this comment.
Moved from .errors import ResourceException at the top
| # The body is the third argument in ResourceException | ||
| if hasattr(e, 'args') and len(e.args) >= 3: | ||
| body = e.args[2] | ||
| if callback: |
There was a problem hiding this comment.
Why is the callback being checked here?
There was a problem hiding this comment.
the callback check was unnecessary since _make_request() already handles callbacks. I've removed it. Now the method simply returns e.body directly when it's a valid zonefile response.
| # If it's about invalid JSON, that's expected - extract the body | ||
| if "invalid json in response" in str(e): | ||
| # The body is the third argument in ResourceException | ||
| if hasattr(e, 'args') and len(e.args) >= 3: |
There was a problem hiding this comment.
ResourceException is a custom error, you can access the body like:
body = e.body
invalid json in response is too generic, since any http/txt response will probably result in this error too and this will just assume it's a zonefile.
Can you update this to check that the response code is valid otherwise raise the error.
And to check that the content type header is valid too.
| # 1. Initiate the export job | ||
| # 2. Poll the status until complete or failed | ||
| # 3. Download and return the zone file content | ||
| zone = api.loadZone("example.com") |
There was a problem hiding this comment.
Can you set example.com to be a variable and then use that for the script, it's a bit neater for using it.
There was a problem hiding this comment.
Made the output filename dynamic using f"{zone_name}.txt"
hhellyer
left a comment
There was a problem hiding this comment.
Code looks good, I think there's one improvement/simplification you can make and we probably ought to get the copyright statement correct but no major issues!
hhellyer
left a comment
There was a problem hiding this comment.
LGTM - Thanks for making changes.
|
@soniafrancisNS1 can you have a look at #150 and check how it integrates with this PR? |
|
They are 2 ways to export zone:
The name of the example |
cmckenna-ibm
left a comment
There was a problem hiding this comment.
LGTM, I see my comments have been resolved, thank you @soniafrancisNS1.
* Allow parameter export=True for zone export With the parameter export=True the output of zone export looks differently: - key `short_answers` becomes 'answers' - key 'id' is removed - key 'zone_name' is removed * Reformat code with black * Pass params to _make_request() only if it's not None * Rename example to avoid filename collision with #145
| # Save to a file | ||
| output_file = f"{zone_name}.txt" | ||
| with open(output_file, "w") as f: | ||
| with open("example.com.txt", "w") as f: |
There was a problem hiding this comment.
output_file variables was created from zone_name. Now the filename is hardcoded on 2 places, variable zone_name is not used when it should be.
There was a problem hiding this comment.
Ah, too late 😞 Thanks for catching that; I thought the file had been renamed too. Let's fix it in the next release.
Allows customers to export zones in BIND format for backup/migration.